Divide project into library and binary parts #112

tuguzT · 2025-09-18T17:18:45Z

Fixes #105

tombh · 2025-09-18T21:08:43Z

crates/cargo-gpu/Cargo.toml

+
+[features]
+watch = ["spirv-builder/watch"]
+clap = ["dep:clap", "spirv-builder/clap"]


Would a little comment here describing the clap-the-feature, be useful here?

Yes, I agree a little note would be good.

tombh · 2025-09-18T21:08:51Z

So this allows using cargo-gpu as a library? I wonder if there is simple e2e test for that? And/or perhaps an example in the README of how to do that?

Cargo.toml

schell · 2025-09-18T21:41:16Z

crates/cargo-gpu-cache/src/build.rs

@@ -89,6 +101,7 @@ impl Build {
            std::env::current_dir()?.display()
        );

+        #[cfg(feature = "watch")]


We need to restructure this to handle the case where the watch feature is disabled to let the else case happen. I'm pretty sure that with this change if the watch feature is missing the entire if else won't be compiled.

Something like:

#[cfg(feature = "watch")] let watching = self.build.watch; #[cfg(not(feature = "watch"))] let watching = false; if watching { ... } else { ... }

I agree with that, I'll rewrite this code soon.

The main difficulty here is not the flag itself, but the code inside of is watching block, which is valid only if watch feature of spirv-builder is enabled.
I really want to move this logic inside of private method to not to clutter run method with conditional compilation handling.

schell · 2025-09-18T21:47:25Z

crates/cargo-gpu/src/build.rs

        Ok(())
    }

    /// Parses compilation result from `SpirvBuilder` and writes it out to a file
+    #[cfg(feature = "watch")]


I don't think we want this cfg here, because we still want to parse compilation results if we're not watching.

I placed this just because this was marked by linter as unused (because I've cfg-featured the code block above).
Rewriting an if above with removing this cfg will resolve it.

crates/cargo-gpu/src/lib.rs

schell

Just needs a few tweaks to the cfg stuff. Good work!

nazar-pc · 2025-09-18T22:58:57Z

So this allows using cargo-gpu as a library? I wonder if there is simple e2e test for that? And/or perhaps an example in the README of how to do that?

It is already possible on main. This PR just allows you to not compile binary-only dependencies (like clap) when you only use it as a library, see linked issue.

crates/cargo-gpu/src/build.rs

tuguzT · 2025-09-20T07:36:57Z

crates/cargo-gpu-cache/src/build.rs

                    if let Some(accept) = accept {
-                        accept.submit(result1);
+                        accept.submit(parse_result);
                    }
                })?
                .context("unreachable")??;


I have a strong feeling that this line means watch method should return ! or anyhow::Result<!>, but I'm not sure which one was in mind when this code were written.

Without looking at the code I'm thinking it makes most sense to use anyhow::Result<!>. Conceptually, watching is a never-ending loop, and there could be errors along the way.

Ah, I see, that's on SpirvBuilder...

So it looks like SpirvBuilder spawns a thread and calls the given callback with the compilation result. But it doesn't look like there's any way to know when/if that spawned thread exits. It seems expected we loop forever until the user exits explicitly (hits control-c), so I don't think SpirvBuilder::watch needs to communicate anything, and I think we can provide the anyhow::Result<!> in our watch function by doing:

loop { std::thread::park(); }

@tombh, @Firestar99 how do you two feel about this?

Even though I originally added the watch code in cargo-gpu I don't remember all of the failure cases I'm afraid. I do personally use --watch often and it works well (just a small annoyance that it compiles twice on first run). So all I'd want to see is that the compilation logs still display and that CTRL+C exits.

I think in practice the loop above should never actually loop, as thread::park should never return. But the loopgives us the return type we want.

I haven't actually written any of this code I'm suggesting though, so the real compiler (as opposed to my head compiler) might have something else to say about it ;).

I noticed that if shader crate compiles with errors in the first time, it returns an error, so it cannot be anyhow::Result<!>.

I also decided to turn panics into errors.
I removed loop and thread park entirely: I don't know if this behavior is Windows-specific, but watching continued without explicit waiting.

Additionally, I have a feeling that watch could return JoinHandle or its wrapper to the caller. Then we could just join it inside of this private method and return an error if watching ends somehow.
~~This is impossible to do, because JoinHandle<!> cannot be returned.~~ But we can return Infallible, which hopefully will be the never type after its stabilization.

I've made a PR for that.

tuguzT · 2025-09-20T07:42:48Z

Also I'm not sure about APIs of library and binary parts:

Should Cli, Command structs and some binary-only features (for example, Config and dump usage) be moved inside of binary part?
What should the minimal API of library part be (for it to be usable inside of a build script)?
Is it just all the pub contents of install module & spirv-builder re-export (as it is currently), or is there something else what could be useful inside of a build script?

This could be resolved in some separate PR, but I think this needs to be addressed at least.

tuguzT · 2025-09-21T17:07:33Z

I'll try to reorder commits of this branch via rebase now to de-clutter merge history.

P.S.: success! 🎊

tuguzT · 2025-09-21T17:39:27Z

So this allows using cargo-gpu as a library? I wonder if there is simple e2e test for that? And/or perhaps an example in the README of how to do that?

It is already possible on main. This PR just allows you to not compile binary-only dependencies (like clap) when you only use it as a library, see linked issue.

I think it would be nice to add build.rs script for shader-crate-template showing the usage of cargo-gpu as a library, but should this be done here or in a separate PR?

nazar-pc · 2025-09-21T17:42:51Z

Certainly a separate PR

schell

So far so good, we just need to address the feature comment and the watch situation. Thank you!

Firestar99 · 2025-09-21T21:06:20Z

Wait what, how did I miss this one? Will have a look tomorrow morning.
(I should probably be on the default-reviewer list as well)

Firestar99

One huge red flag: We cannot rename the cli tool. It must stay cargo-gpu, otherwise you'd have to call it with cargo gpu-cli instead of just cargo gpu (iirc). Especially since we want to (more strongly) mirror cargo commands in the future, like cargo gpu check or cargo gpu clippy.

Otherwise, I strongly support the split up. I just haven't gotten around to it, or figured out a good alternative name for the "cargo-gpu-lib". Some ideas, feel free to suggest more:

rustc_codegen_spirv_cache
cargo_gpu_cache
rust_gpu_cache

nazar-pc · 2025-09-21T21:43:39Z

One huge red flag: We cannot rename the cli tool.

The binary is still cargo-gpu (see [[bin]] section), only the crate name has changed.

schell · 2025-09-22T04:15:32Z

Also I'm not sure about APIs of library and binary parts:

* Should `Cli`, `Command` structs and some binary-only features (for example, `Config` and dump usage) be moved inside of binary part?

* What should the minimal API of library part be (for it to be usable inside of a build script)?
  Is it just all the pub contents of `install` module & `spirv-builder` re-export (as it is currently), or is there something else what could be useful inside of a build script?

This could be resolved in some separate PR, but I think this needs to be addressed at least.

I would just make most of it pub and call it a day. We can always go back and clean up the API if it's too complex.

nazar-pc · 2025-09-22T04:23:41Z

In case it is useful as a reference, here is the minimal API I'm using right now:

        let backend = cargo_gpu::Install::from_shader_crate(shader_crate.clone()).run()?;

        let spirv_builder = backend
            .to_spirv_builder(shader_crate, "spirv-unknown-vulkan1.2")

https://github.com/nazar-pc/abundance/blob/f8ec952de139c87509a5080c71142e43f351b3e9/crates/farmer/ab-proof-of-space-gpu/build.rs#L37-L40

Plus spirv_builder crate re-export so I don't have to guess its version.

P.S. Noticed just now that for some reason path to shader crate (shader_crate) is needed twice, feels redundant 🤷

The goal in the issue #105 was to remove dependencies from the library crate. This PR is a step in the right direction, which makes dependencies optional. But it still doesn't remove them, which causes interesting side-effects when Cargo's feature unification kicks in. So I'd say eventually those dependencies should be removed, which likely means all data structures that depend on them (like CLI arguments/options) will need to move as well.

tuguzT · 2025-09-22T07:47:46Z

One huge red flag: We cannot rename the cli tool. It must stay cargo-gpu, otherwise you'd have to call it with cargo gpu-cli instead of just cargo gpu (iirc). Especially since we want to (more strongly) mirror cargo commands in the future, like cargo gpu check or cargo gpu clippy.

Otherwise, I strongly support the split up. I just haven't gotten around to it, or figured out a good alternative name for the "cargo-gpu-lib". Some ideas, feel free to suggest more:

rustc_codegen_spirv_cache

cargo_gpu_cache

rust_gpu_cache

I honestly do not want to rename cargo-gpu crate into cargo-gpu-cli too (and binary name cargo-gpu must remain the same, as it is in this PR), but I haven't got any ideas about naming of library part.
Am I right to assume that cache in the suggested names means caching of fixed nightly toolchain and codegen shared library?

tuguzT · 2025-09-22T07:56:15Z

In case it is useful as a reference, here is the minimal API I'm using right now:
let backend = cargo_gpu::Install::from_shader_crate(shader_crate.clone()).run()?;

let spirv_builder = backend
    .to_spirv_builder(shader_crate, "spirv-unknown-vulkan1.2")
https://github.com/nazar-pc/abundance/blob/f8ec952de139c87509a5080c71142e43f351b3e9/crates/farmer/ab-proof-of-space-gpu/build.rs#L37-L40

Plus spirv_builder crate re-export so I don't have to guess its version.

Inside of my project (which I mentioned in #93) I moved to cargo-gpu library as soon as it became possible to do, because std's Command API worked only sometimes.
I use similar symbols, too (namely cargo_gpu::Install and spirv_builder reexport).

schell · 2025-09-22T18:55:28Z

So long as cargo-gpu works for the binary name, and you said it did, then cargo-gpu-cli is fine for the crate name. It's a nice and descriptive name, and folks won't really interact with it anyway.

Thanks for your great work @tuguzT!

tuguzT · 2025-09-22T20:24:30Z

Some ideas, feel free to suggest more:

rustc_codegen_spirv_cache

cargo_gpu_cache

rust_gpu_cache

I think rustc_codegen_spirv_cache or spirv-builder-cache might suite quite well.

The former suits because, as stated in README,

cargo gpu compiles a custom codegen backend for rustc

The latter - because cargo-gpu directly re-exports spirv-builder and enhances its capabilities by managing its codegen.

P.S.: also these names could contain toolchain in them to signal that the library manages toolchain installation.

But for now, I prefer rustc_codegen_spirv_cache over other variants.

tuguzT · 2025-09-23T12:09:01Z

Another point against cargo-gpu-cli with its binary renamed to cargo-gpu is the following issue with documentation:

warning: output filename collision.
The bin target `cargo-gpu` in package `cargo-gpu-cli v0.1.0 (C:\...\cargo-gpu\crates\cargo-gpu-cli)` has the same output filename as the lib target `cargo_gpu` in package `cargo-gpu v0.1.0 (C:\...\cargo-gpu\crates\cargo-gpu)`.
Colliding filename is: C:\...\cargo-gpu\target\doc\cargo_gpu\index.html
The output filenames should be unique.
This is a known bug where multiple crates with the same name use
the same path; see <https://github.com/rust-lang/cargo/issues/6313>.
If this looks unexpected, it may be a bug in Cargo. Please file a bug report at
https://github.com/rust-lang/cargo/issues/ with as much information as you
can provide.
cargo 1.90.0 (840b83a10 2025-07-30) running on `x86_64-pc-windows-msvc` target `x86_64-pc-windows-msvc`
First unit: Unit { pkg: Package { id: PackageId { name: "cargo-gpu-cli", version: "0.1.0", source: "C:\\...\\cargo-gpu\\crates\\cargo-gpu-cli" }, ..: ".." }, target: TargetInner { name: "cargo-gpu", doc: true, ..: with_path("C:\\...\\cargo-gpu\\crates\\cargo-gpu-cli\\src\\main.rs", Edition2021) }, profile: Profile { ..: default_dev() }, kind: Host, mode: Doc, features: [], rustflags: [], rustdocflags: [], links_overrides: {}, artifact: false, artifact_target_for_features: None, is_std: false, dep_hash: 755822672195230307 }
Second unit: Unit { pkg: Package { id: PackageId { name: "cargo-gpu", version: "0.1.0", source: "C:\\...\\cargo-gpu\\crates\\cargo-gpu" }, ..: ".." }, target: TargetInner { name_inferred: true, ..: lib_target("cargo_gpu", ["lib"], "C:\\...\\cargo-gpu\\crates\\cargo-gpu\\src\\lib.rs", Edition2021) }, profile: Profile { ..: default_dev() }, kind: Host, mode: Doc, features: ["clap", "watch"], rustflags: [], rustdocflags: [], links_overrides: {}, artifact: false, artifact_target_for_features: None, is_std: false, dep_hash: 15945473915602456875 }

This means that cargo-gpu library docs could be inacessible, because its doc page could be rewritten with cargo-gpu-cli binary doc page, and vice versa.

Firestar99 · 2025-09-29T10:44:34Z

Have you changed something CI related that lints is failing, or is it just failing in general? (I'll go fix it if it's on us)

tuguzT · 2025-09-29T16:36:21Z

I've moved reworked, more aggressive lib/bin split in a separate draft, #117

tuguzT force-pushed the split-lib-bin-deps branch 2 times, most recently from 9e75b95 to 2c4c4bd Compare September 18, 2025 18:35

tuguzT marked this pull request as ready for review September 18, 2025 20:50

tombh reviewed Sep 18, 2025

View reviewed changes

schell reviewed Sep 18, 2025

View reviewed changes

Cargo.toml Show resolved Hide resolved

schell reviewed Sep 18, 2025

View reviewed changes

crates/cargo-gpu/src/lib.rs Show resolved Hide resolved

schell requested changes Sep 18, 2025

View reviewed changes

tuguzT requested a review from schell September 19, 2025 08:51

tuguzT commented Sep 19, 2025

View reviewed changes

crates/cargo-gpu/src/build.rs Outdated Show resolved Hide resolved

tuguzT commented Sep 20, 2025

View reviewed changes

tuguzT force-pushed the split-lib-bin-deps branch from a29f17d to b541ec6 Compare September 21, 2025 17:30

schell requested changes Sep 21, 2025

View reviewed changes

Firestar99 requested changes Sep 21, 2025

View reviewed changes

This comment was marked as off-topic.

Sign in to view

tuguzT force-pushed the split-lib-bin-deps branch from b541ec6 to d3026f8 Compare September 22, 2025 19:55

This comment was marked as outdated.

Sign in to view

tuguzT force-pushed the split-lib-bin-deps branch 5 times, most recently from 7dce50f to a01cb15 Compare September 24, 2025 20:36

tuguzT mentioned this pull request Sep 25, 2025

Return JoinHandle of watching thread to the caller Rust-GPU/rust-gpu#413

Merged

This comment was marked as outdated.

Sign in to view

tuguzT added 4 commits September 26, 2025 12:38

Divide project into library and binary parts

43979c6

Add clap feature for cargo-gpu library

4374ada

Add watch feature for cargo-gpu library

c4613d7

Fix incorrect watch feature handling in build

30747ac

tuguzT force-pushed the split-lib-bin-deps branch 8 times, most recently from c9419ef to 89d5713 Compare September 28, 2025 08:16

tuguzT mentioned this pull request Sep 28, 2025

Debug for CargoCmd Rust-GPU/rust-gpu#415

Merged

tuguzT force-pushed the split-lib-bin-deps branch from ababc64 to 8954b47 Compare September 28, 2025 15:11

This comment was marked as outdated.

Sign in to view

tuguzT mentioned this pull request Sep 29, 2025

Make AcceptFirstCompile public Rust-GPU/rust-gpu#417

Closed

tuguzT mentioned this pull request Sep 29, 2025

Divide project into library and binary parts, round 2 #117

Open

tuguzT force-pushed the split-lib-bin-deps branch from 76282f1 to 30747ac Compare October 1, 2025 13:08

Divide project into library and binary parts #112

Are you sure you want to change the base?

Divide project into library and binary parts #112

Uh oh!

Conversation

tuguzT commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tombh commented Sep 18, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schell left a comment

Choose a reason for hiding this comment

Uh oh!

nazar-pc commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tuguzT Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuguzT Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuguzT commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tuguzT commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tuguzT commented Sep 21, 2025

Uh oh!

nazar-pc commented Sep 21, 2025

Uh oh!

schell left a comment

Choose a reason for hiding this comment

Uh oh!

Firestar99 commented Sep 21, 2025

Uh oh!

Firestar99 left a comment

Choose a reason for hiding this comment

Uh oh!

nazar-pc commented Sep 21, 2025

Uh oh!

schell commented Sep 22, 2025

Uh oh!

nazar-pc commented Sep 22, 2025

Uh oh!

tuguzT commented Sep 22, 2025

Uh oh!

tuguzT commented Sep 22, 2025

tuguzT commented Sep 18, 2025 •

edited

Loading

nazar-pc commented Sep 18, 2025 •

edited

Loading

tuguzT Sep 20, 2025 •

edited

Loading

tuguzT Sep 25, 2025 •

edited

Loading

tuguzT commented Sep 20, 2025 •

edited

Loading

tuguzT commented Sep 21, 2025 •

edited

Loading

tuguzT commented Sep 22, 2025 •

edited

Loading

tuguzT commented Sep 23, 2025 •

edited

Loading

tuguzT commented Sep 29, 2025 •

edited

Loading